Skip to content

Conversation

@ranj063
Copy link
Collaborator

@ranj063 ranj063 commented Oct 25, 2025

Preparatory patches with updates to the tone module and topology support for the tone and pipeline direction attribute.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds support for hostless pipelines using a tone generator as an example. The changes enable pipelines to run without a host connection by introducing direction awareness and implementing the tone generator as a modular component.

Key changes:

  • Implemented pipeline direction tracking in IPC4 to support hostless operation
  • Refactored tone generator from legacy component to module adapter architecture
  • Added configuration support for tone generator in SDW platform topologies

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tools/topology/topology2/platform/intel/sdw-amp-generic.conf Adds tone generator pipeline configuration with PCM definition and routing
tools/topology/topology2/include/pipelines/virtual-siggen-mixin-playback.conf Defines new virtual signal generator to mixer pipeline for tone playback
tools/topology/topology2/include/components/virtual.conf Adds stream_name attribute to virtual widget component
tools/topology/topology2/include/components/siggen.conf New signal generator component definition with configuration attributes
tools/topology/topology2/cavs-sdw.conf Includes new pipeline and adds SPK_TONE_PLAYBACK configuration flag
src/ipc/ipc4/helper.c Extracts and stores pipeline direction from IPC extension data
src/include/sof/audio/pipeline.h Adds direction tracking fields to pipeline structure
src/include/ipc4/pipeline.h Defines direction bitfields in IPC4 pipeline extension structure
src/audio/tone.toml Adds module configuration manifest for tone generator
src/audio/tone.c Refactors tone generator to module adapter interface with sink API
src/audio/module_adapter/module_adapter.c Propagates pipeline direction to module devices

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

# A generic siggen widget. All attributes defined herein are namespaced
# by alsatplg to "Object.Widget.siggen.N.attribute_name"
#
# Usage: this component can be used by declaring int a parent object. i.e.
Copy link

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'int' to 'in'.

Suggested change
# Usage: this component can be used by declaring int a parent object. i.e.
# Usage: this component can be used by declaring in a parent object. i.e.

Copilot uses AI. Check for mistakes.
# Attribute categories
attributes {
#
# The sighen widget name would be constructed using the index and instance attributes.
Copy link

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected spelling of 'sighen' to 'siggen'.

Suggested change
# The sighen widget name would be constructed using the index and instance attributes.
# The siggen widget name would be constructed using the index and instance attributes.

Copilot uses AI. Check for mistakes.
src/audio/tone.c Outdated
int ret;

/* tone generator only ever has 1 sink */
output_frames = sink_get_free_frames(sinks[0]);
Copy link

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent array indexing. Line 395 uses sinks[0] while line 389 assigns sink = sinks[0]. Consider using the sink variable consistently throughout the function (e.g., sink_get_free_frames(sink)).

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question on design.

if (dev->pipeline->direction_set) {
dev->direction_set = true;
dev->direction = dev->pipeline->direction;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, ipc_comp_connect can propagates direction both ways, so shouldn't it be possible to set component directions based on "DAI copier"? The direction is passed to DAI copier already from host "get_gateway_direction(node_id.f.dma_type)", so direction is known.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i ipc_comp_connect checks both sides but the trigger is always propagated from source->sink. So if there's no host copier, the tone pipeline wont have a source direction to hinge upon

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 Right but can we extend this? Seems FW has enough information to do the right thing without any new information from host (even if it doesn't do it now).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i I dont want to extend it at all, in fact Its quite a convoluted logic having to walk through the entire pipeline just to set the component's direction. This should be simplified

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 Couldn't this module be converted into some virtual/fake DAI and replace the host copier with it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 Ack. I guess this is more inline with the IPC4 philosophy of letting host provide all programming. It does still feel a bit redundant to me. The whole notion of direction is a bit redundant in describing a graph when you already provide have a description of a graph and how nodes are connected. It seems only entities that have some special functionality around playback/capture are the copier/endpoint modules and for these there's already a direction flag passed (e.g. in copier config blob). So if "direction" is only related to the endpoints, looking up the direction by traversin the graph topology, does not seem so convoluted.

Comment on lines 108 to 110
uint32_t core_id : 4;
uint32_t rsvd2 : 6;
uint32_t direction_set : 1;
uint32_t direction : 1;
uint32_t rsvd2 : 4;
uint32_t _reserved_2 : 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in these structures require prior synchronization and acceptance from the architecture side.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tmleman ack. How do I go about this?

uint32_t time_domain; /**< scheduling time domain */
uint32_t attributes; /**< pipeline attributes from IPC extension msg/ */
uint32_t direction_set; /**< flag indicating if direction set from IPC extension msg? */
uint32_t direction; /**< pipeline direction from IPC extension msg */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bool

src/audio/tone.c Outdated
dev->ipc_config = *config;
comp_info(dev, "tone_new()");

cd = rzalloc(SOF_MEM_FLAG_USER, sizeof(*cd));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mod_alloc() while at it to save @jsarha some work :-)

static SHARED_DATA struct comp_driver_info comp_tone_info = {
.drv = &comp_tone,
};
#if CONFIG_COMP_TONE_MODULE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it isn't tristate yet, it's boolean, LLEXT isn't supported for the tone generator yet, no need for this branch yet

@@ -0,0 +1,21 @@
#ifndef LOAD_TYPE
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file is probably unneeded yet - nobody includes it

#
## Stream name - maps to the DAPM widget's stream name
#
DefineAttribute."stream_name" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be virtual_stream_name ? i.e. if if for virtual usage only we best name/state that so its used correctly.

@kv2019i kv2019i dismissed their stale review October 29, 2025 14:12

The need to pass direction was discussed, I don't have strong opposition on either path, just wanted to check the options have been considered.

@ranj063 ranj063 force-pushed the fix/hostless_pipelines branch from 89e64f3 to 2ee7695 Compare November 21, 2025 00:31
@ranj063 ranj063 changed the title Add support for hostless pipelines (ex: tone generator) Add support for hostless/dailess pipelines Nov 21, 2025
@ranj063
Copy link
Collaborator Author

ranj063 commented Nov 21, 2025

@kv2019i @singalsu Ive dropped the extension of the pipeline IPC message and used the pin logic in the tone module to set the direction now.

static void tone_s32_default(struct comp_dev *dev, struct audio_stream *sink,
uint32_t frames)
static int tone_s32_passthrough(struct processing_module *mod, struct sof_sink *sink,
struct sof_source *source)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does a tone generator have to support pass-through? Was it also possible with IPC3?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tone generator operates as silence generator in this case, when there is no playback happening. Silence is a kind of audio signal too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh with IPC3 we didnt have firmware based echo reference feature. This new addition to do passthrough is required for supporting that with IPC4 for SDW topologies.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tone generator operates as silence generator in this case, when there is no playback happening. Silence is a kind of audio signal too.

@singalsu interesting, thanks. I think we also have ad-hoc silence generation in some components. Is it planned to remove those and use "tone" in all cases?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd assume those silence generator capabilities remain like now. E.g. mixin/mixout operation is defined by IPC4. While with this component with no Windows OS dependence we can define it as we need.

Copy link
Member

@lgirdwood lgirdwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 feel free to fix the spelling in a subsequent PR if CI passes


static void tone_s32_default(struct comp_dev *dev, struct audio_stream *sink,
uint32_t frames)
static int tone_s32_passthrough(struct processing_module *mod, struct sof_sink *sink,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use for passthrough the helper source_to_sink_copy(). There's an example in

source_to_sink_copy(source, sink, true, frames * cd->frame_bytes);

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this as a future optimzation @singalsu

@dbaluta
Copy link
Collaborator

dbaluta commented Nov 24, 2025

@ranj063 I wonder how difficult for your change would be to keep the support for IPC3. We want to add this feature to our release too but IMX doesnt support IPC4 yet.

@ranj063
Copy link
Collaborator Author

ranj063 commented Dec 3, 2025

@ranj063 I wonder how difficult for your change would be to keep the support for IPC3. We want to add this feature to our release too but IMX doesnt support IPC4 yet.

@dbaluta I've left the ipc3 version as is now and created a new implementation for IPC4

@ranj063 ranj063 force-pushed the fix/hostless_pipelines branch from 99f1100 to a94e307 Compare December 3, 2025 19:22
n_wrap_dest = output_end - output_pos;

/* Process until wrap or completed n */
n_min = (n < n_wrap_dest) ? n : n_wrap_dest;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here a MIN() would fit perfectly

/* sg->w is angle in Q4.28 radians format, sin() returns Q1.31 */
/* sg->a is amplitude as Q1.31 */
sine =
q_mults_32x32(sin_fixed_32b(sg->w), sg->a,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge with previous line please


if (sg->mute)
return 0;
else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else not needed

int64_t w;
/* sg->w is angle in Q4.28 radians format, sin() returns Q1.31 */
/* sg->a is amplitude as Q1.31 */
sine =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's checkpatch that has a habit of complaining about missing empty lines between variable declarations and code, not sure if comments make it happy though

sg->f = (f > f_max) ? f_max : f;
/* Q16 x Q31 -> Q28 */
w_tmp = q_multsr_32x32(sg->f, sg->c, Q_SHIFT_BITS_64(16, 31, 28));
w_tmp = (w_tmp > PI_Q4_28) ? PI_Q4_28 : w_tmp; /* Limit to pi Q4.28 */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MIN()

int i;

sg->a_target = a;
sg->a = (sg->ramp_step > sg->a_target) ? sg->a_target : sg->ramp_step;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MIN()

*/
for (i = 0; i < TONE_NUM_FS; i++) {
if (fs == tone_fs_list[i])
idx = i;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

break?


comp_info(dev, "tone_new()");

cd = rzalloc(SOF_MEM_FLAG_USER, sizeof(*cd));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mod_alloc()

struct comp_data *cd = module_get_private_data(mod);
int i;

comp_info(mod->dev, "tone_reset()");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please check and remove all function names from logs, also please try to reduce the number of *_info() level logs, e.g. this one doesn't seem to be really important

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh all of these comments above are on existing code. I will fix the mod_alloc() but Im going to leave the rest for now for the sake of consistency with the xisting code in tone.c. We can clean it up later

};

#if CONFIG_COMP_TONE_MODULE
/* modular: llext dynamic link */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

again, unless you're making it tristate, this isn't needed, as well as the .toml file below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lyakh @singalsu requested to keep the toml file as he is testing with it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ranj063 then do you maybe have the remaining code for making it a real LLEXT? Not much is left - "tristate" in Kconfig, a couple of simple cmake changes, that can be copied from plentiful examples?

Add a new implementation for the tone modeule for IPC4 with no kcontrol
support as of now. This will be added later.

Signed-off-by: Ranjani Sridharan <[email protected]>
Create a new conf file for the siggen module.

Signed-off-by: Ranjani Sridharan <[email protected]>
Add 2 new tokens for pipeline direction and atttributes for the
pipeline component. This change is required to implement the firmware
based echo reference feature to inform the kernel of the pipeline
direction in order for it to make a decision on which pipelines to set
up when the echo reference capture pipeline is set up. The
direction_valid attribute is required for backward-compatibility in the
kernel driver for older topologies where the direction is not set.

Signed-off-by: Ranjani Sridharan <[email protected]>
@ranj063 ranj063 force-pushed the fix/hostless_pipelines branch from a94e307 to 876c06c Compare December 8, 2025 05:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants